-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BE-586 | Claimbot #524
base: v26.x
Are you sure you want to change the base?
BE-586 | Claimbot #524
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - nice work.
Once the solution is cleaned up with tests added, could you please also link a few DataDog trace samples of processing the block as claimbot?
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
Cleans up OrderBookClient by reusing slices.Split instead of duplicating splitting slices into chunks logic in some of the methods.
Fixes errors running fillbot via docker-compose
WalkthroughThe pull request introduces a series of modifications across multiple files to enhance the functionality of orderbook plugins, specifically focusing on the addition of an "Orderbook Claim Bot." Key changes include updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
}, | ||
``` | ||
|
||
Configure the key on a test keyring, and set the following environment variables: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for lack of context here, key is named as local
but then what is meant by local.info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's full name of the key on the disk as per my understanding, if you would list contents you would see something like that:
ll ~/.osmosisd/keyring-test/
total 16
drwxr-xr-x 2 root root 4096 Oct 25 13:54 ./
drwxr-xr-x 7 root root 4096 Oct 25 13:40 ../
-rw------- 1 root root 537 Oct 25 13:54 abcd....address
-rw------- 1 root root 735 Oct 25 13:54 local.info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for lack of context, should we consider any parallelization here? Guess we shouldn't due to business requirements and so I'm just asking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is possible to apply parallelization for some calls, for example for procssing different orderbooks. Let me keep this conversation open, and let's see what @p0mvn thinks. In case we decide to do that I think we should create separate task for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with the direction of creating a separate task. This should be an easy win that allows us to avoid worrying about any processing latencies in the future.
Let's tackle it before completing the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work! Please consider the comments left. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
log/logger.go (1)
40-43
: Fix incorrect method documentationThe comment "Warn implements Logger" is incorrect for the Named method.
-// Warn implements Logger. +// Named implements Logger. func (l *NoOpLogger) Named(s string) Logger {ingest/usecase/plugins/orderbook/claimbot/plugin.go (3)
36-40
: ConvertblockInclusionWaitTime
to a constant.Since
blockInclusionWaitTime
is a fixed value, it should be defined as a constant alongside other constants for better maintainability and clarity.Apply this diff:
const ( tracerName = "sqs-orderbook-claimbot" + blockInclusionWaitTime = 5 * time.Second ) var ( tracer = otel.Tracer(tracerName) fillThreshold = osmomath.MustNewDecFromStr("0.98") - blockInclusionWaitTime = 5 * time.Second )
153-154
: Document the purpose ofblockInclusionWaitTime
.While the sleep is necessary, its purpose should be better documented to explain why this specific duration was chosen and what issues it prevents.
Add a detailed comment explaining the rationale:
- // Wait for block inclusion with buffer to avoid sequence mismatch + // Wait for block inclusion with a 5-second buffer to ensure the previous transaction + // is included in a block before sending the next one. This prevents sequence mismatch + // errors that can occur when transactions are sent too quickly in succession. time.Sleep(blockInclusionWaitTime)
23-28
: Document plugin behavior during edge cases.The plugin documentation should include information about:
- How it handles chain upgrades
- Behavior during network connectivity issues
- Recovery mechanisms for failed transactions
- Any assumptions about chain block times relative to
blockInclusionWaitTime
This will help operators understand the plugin's limitations and requirements.
Add comprehensive documentation to the struct:
-// claimbot is a claim bot that processes and claims eligible orderbook orders at the end of each block. -// Claimable orders are determined based on order filled percentage that is handled with fillThreshold package level variable. +// claimbot is a claim bot that processes and claims eligible orderbook orders at the end of each block. +// Claimable orders are determined based on order filled percentage that is handled with fillThreshold package level variable. +// +// Edge Cases: +// - Chain Upgrades: The plugin will stop processing during upgrades and resume after chain restart +// - Network Issues: Failed transactions will be logged but not retried automatically +// - Block Times: Assumes minimum block time > blockInclusionWaitTime to prevent transaction sequence issues +// - Recovery: Manual intervention may be required if transactions fail consistently
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- ingest/usecase/plugins/orderbook/claimbot/config.go (1 hunks)
- ingest/usecase/plugins/orderbook/claimbot/plugin.go (1 hunks)
- log/logger.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ingest/usecase/plugins/orderbook/claimbot/config.go
🔇 Additional comments (3)
log/logger.go (3)
11-11
: LGTM: Named logger interface additionThe addition of the
Named
method to the Logger interface follows standard logging patterns and enables hierarchical logging contexts.
128-130
: LGTM: Return type change promotes better abstractionThe change to return the Logger interface instead of concrete type is a good practice that promotes loose coupling.
11-11
: Verify logger usage patterns across the codebaseThe addition of the Named method and changes to logger instantiation might require updates in other parts of the codebase.
Also applies to: 73-77, 128-130
✅ Verification successful
Let me gather more information about the logger implementation and its usage patterns.
Based on the comprehensive analysis of the logger usage across the codebase, I can now provide the final response:
The Named method addition is safe and correctly implemented
The Named method is already being used in the codebase, specifically in the claimbot configuration:
ingest/usecase/plugins/orderbook/claimbot/config.go
:Logger: logger.Named("claimbot")
The logger interface and implementation are self-contained within the
log
package, and the Named method is a standard logging pattern for creating scoped loggers. The addition of this method doesn't require any mandatory updates across the codebase as:
- It's an optional method that enhances logger functionality
- Existing logger usage patterns remain unaffected
- The implementation follows the standard logging pattern of creating named/scoped loggers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check logger instantiation and usage patterns # Check for direct zap.Logger usage that might need updating echo "Checking for direct zap.Logger usage..." rg --type go "zap\.Logger" # Check for existing logger naming patterns that might use the new Named method echo "Checking existing logger naming patterns..." rg --type go "logger.*Named\("Length of output: 444
Script:
#!/bin/bash # Check the logger interface and implementation echo "Checking logger interface and implementation..." rg --type go -A 5 "type Logger interface" # Check for all logger instantiations echo "Checking logger instantiations..." rg --type go "New(Logger|ZapLogger)" # Check for all logger usages in the codebase echo "Checking logger usages..." rg --type go "logger\." -A 2Length of output: 29969
func (l *loggerImpl) Named(s string) Logger { | ||
return &loggerImpl{ | ||
zapLogger: *l.zapLogger.Named(s), | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add nil check for zapLogger
The current implementation could panic if zapLogger is nil. Consider adding a nil check for defensive programming.
func (l *loggerImpl) Named(s string) Logger {
+ if l == nil || &l.zapLogger == nil {
+ return &NoOpLogger{}
+ }
return &loggerImpl{
zapLogger: *l.zapLogger.Named(s),
}
}
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Left comment - please take a look
batchClaim := struct { | ||
BatchClaim struct { | ||
Orders [][]int64 `json:"orders"` | ||
} `json:"batch_claim"` | ||
}{ | ||
BatchClaim: struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's address code rabbit rec below
orderbooks, err := getOrderbooks(o.config.PoolsUseCase, blockHeight, metadata) | ||
if err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having this as a function and propagating pools use case as an arg, why not make this be a method on the pools use case?
OrderbookUsecase mvc.OrderBookUsecase | ||
OrderbookRepository orderbookdomain.OrderBookRepository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should really only need the use case. Usecase encapsulates the repository. So all requests to the repository should be proxied through the use case.
This is an implementation of the MVC design pattern where the repository is the model.
OrderBookClient orderbookgrpcclientdomain.OrderBookClient | ||
AccountQueryClient authtypes.QueryClient | ||
TxfeesClient txfeestypes.QueryClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider abstracting all of the chain clients behind a struct
orderbookRepository orderbookdomain.OrderBookRepository, | ||
orderBookClient orderbookgrpcclientdomain.OrderBookClient, | ||
orderbookusecase mvc.OrderBookUsecase, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to my other comment, we should only need to pass orderbook use case and proxy requests to the repository through it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with the direction of creating a separate task. This should be an easy win that allows us to avoid worrying about any processing latencies in the future.
Let's tackle it before completing the feature.
} | ||
|
||
// processOrderbooksAndGetClaimableOrders processes a list of orderbooks and returns claimable orders for each. | ||
func processOrderbooksAndGetClaimableOrders( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing only this function being covered by tests but there are many downstream function of varying branching complexity.
Could we spend some time covering those with tests?
I think it would pay off in the long-term as it would make any possible refactors drastically easier under simpler test coverage
// Wait for block inclusion with buffer to avoid sequence mismatch | ||
time.Sleep(blockInclusionWaitTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider querying the sequence number at the beginning of the block and incrementing it in-memory. This should be fine since we have a lock on block processing. That is, if block X is being processed, block X+1 processing cannot start.
This would drastically speed up processing all orders within a block
type order struct { | ||
Orderbook domain.CanonicalOrderBooksResult | ||
Orders orderbookdomain.Orders | ||
Err error | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This abstraction name is confusing.
This is not an order but rather orderbook with its claimable orders.
I suggest renaming it accordingly as well as the variable identifiers to make the logic more readable.
b959eba
to
d298179
Compare
Quality Gate failedFailed conditions |
This PR introduces The Orderbook Claimbot plugin. Orderbook Claimbot plugin is a plugin that claims filled order book orders.
It scans all active orders for each order book determining which orders have been filled and need to be claimed. At the moment order is said to be claimable if it is filled 98 percent or more. In order for an order book to be processed to claim its active orders it must be canonical as per SQS definition.
At this moment there is a few drawbacks of current implementation that can be improved in future, such as for example claiming orders from multiple orderbooks within single transaction, or using test keyring alternative.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores